test(github): enforce explicit ReadOnlyHint on every mcp.Tool literal#2486
Open
jluocsa wants to merge 2 commits into
Open
test(github): enforce explicit ReadOnlyHint on every mcp.Tool literal#2486jluocsa wants to merge 2 commits into
jluocsa wants to merge 2 commits into
Conversation
Adds a source-level (AST) validation test that walks every non-test Go file in pkg/github and fails if any mcp.Tool composite literal omits Annotations.ReadOnlyHint. The existing TestAllToolsHaveRequiredMetadata can only assert that Annotations is non-nil at runtime: Go cannot distinguish an unset bool field from one explicitly set to false. The new test closes that gap so future read-intent tools cannot silently default to ReadOnlyHint=false, which has caused downstream agents to prompt for human approval on safe read operations. All 97 current mcp.Tool registrations pass. Fault-injected by removing ReadOnlyHint from issue_read and confirmed the test reports the exact file, line, tool name, and reason. Refs github#2483
jluocsa
force-pushed
the
feat/lint-readonly-hint-annotation-2483
branch
from
69ee9bb to
a973416
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a static-analysis Go test to enforce that every mcp.Tool composite literal in pkg/github explicitly sets Annotations.ReadOnlyHint, preventing unintended defaulting to false.
Changes:
- Introduces an AST-based package scan over non-test Go files to find
mcp.Tool{...}literals. - Validates that
Annotationsis present and statically inspectable as amcp.ToolAnnotations{...}literal. - Fails the test with a detailed, file/line/tool-name-specific report when
ReadOnlyHintis not explicitly set.
- Resolve each file's local alias for github.com/modelcontextprotocol/go-sdk/mcp via file.Imports rather than hard-coding the "mcp" qualifier, so the check also covers files that import the SDK under a non-default alias. - Detect positional (unkeyed) composite literals and report a dedicated diagnostic instead of producing misleading "missing field" violations. - Drop the brittle 'expected to discover at least one mcp.Tool literal' assertion: if registrations move behind constructors/factories the AST walker legitimately finds nothing. - Use strconv.Unquote to decode tool-name string literals (handles escapes in interpreted strings); fall back to the raw lexeme on parse error.
pachecocordovamoiseseduardo-byte
approved these changes
May 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a source-level (AST) validation test that statically walks every non-test
.gofile inpkg/github, finds everymcp.Tool{...}composite literal, and fails if any of them omits an explicitAnnotations.ReadOnlyHint:field.Refs #2483 (item 3 in the description: "Add a CI check that fails if any tool registered via
mcp.NewToollacks an explicitReadOnlyHint").Why
The existing
TestAllToolsHaveRequiredMetadataalready enforces non-nilAnnotationsand explicitly documents its limitation:That gap is exactly what #2483 wants closed. The Go runtime cannot tell an unset
boolfrom one explicitly set tofalse, but the AST can. This test surfaces the omission at build time so a future read-intent tool cannot silently default toReadOnlyHint: false— which has triggered downstream agents to prompt for human approval on safe read operations.How
TestAllToolRegistrationsExplicitlySetReadOnlyHintinpkg/github/tools_static_validation_test.go.go/parser+go/ast(stdlib only — no new dependencies).*ast.CompositeLitof typemcp.Tool:Annotations:is present.&mcp.ToolAnnotations{...}literal that can be statically inspected (anything else is flagged so a human can confirm).ReadOnlyHint:is among the keyed fields.<file>:<line> tool=<name>: <reason>.Verification
go test ./pkg/github -run TestAllToolRegistrationsExplicitlySetReadOnlyHint -v→ PASS (97/97 current tool literals are compliant).ReadOnlyHint: truefromissue_read:go test ./...→ all green.go vet ./pkg/github/...→ clean.Scope notes
This PR is intentionally minimal: it covers item 3 of #2483 (the CI guardrail). The audit portion of the issue (items 1–2) is currently a no-op against
mainbecause all 97 registrations already set the hint, so the value of this change is purely forward-protection. Happy to fold in additional checks (e.g., explicitTitle:annotation, OWASP-flavored side-effect classification) as follow-ups if maintainers want them.